Submodules! #312
Submodules! #312
Conversation
| var submodule = repo.Submodules[submodulePath]; | ||
|
|
||
| var statusBefore = submodule.RetrieveStatus(); | ||
| Assert.Equal(SubmoduleStatus.WorkDirModified, statusBefore & SubmoduleStatus.WorkDirModified); |
There was a problem hiding this comment.
There are other tests where we could use HasFlag() (or HasAny<>()) too. From a purity standpoint, they really don't belong as part of the public surface area of a Git library, but is that a good enough reason? Or there's the slippery slope of [InternalsVisibleTo], which would allow Core.Epoch and the public classes in Core.Compat to be hidden.
|
New set of commits based on latest, with proper tests. First three commits could be merged early if they look good - nothing submodule specific. Next up: immutability. I'd appreciate some feedback on other scenarios everyone would like to see tests for. |
|
I'm using this fork in an evaluation project and it works quite well. I would appreciate it being merged :) |
|
However, I've got a question: is cloning repositories recursively already supported? When I clone a repository, and then call |
@nulltoken How about I strip out the parts related to mutation for now and hit that as a separate PR? Enumeration, lookup and staging seem reasonably solid...
I've not tried using |
| @@ -0,0 +1,169 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
Please make sure we have the proper permission to accept 3rd party code here (or remove the hooks if they are not necessary?).
There was a problem hiding this comment.
I'll kill them.
Related: libgit2/libgit2@fc6c5b5
There was a problem hiding this comment.
See 49ce35b (which can be cherry-picked standalone) and updated 69da28c
|
I've broken the existing mutation members out into their own commit, though I've left all the bindings. If everything else looks satisfactory, I propose we close out this PR and continue with mutation and add/init/update in separate PRs. |
|
Another update on latest, with a new set of properties: Works on my machine, but Travis build is failing for the two tests that expect a I believe this exposes everything one would need to manually clone/update a Another question I had while learning the libgit2 APIs a bit better... Is there any way to tell if |
Looks like d082a81 worked; rebased accordingly. |
|
I think you should also support |
|
I found that |
@dahlbyk Sounds like we should change that part of submodule status to make it clear. I know of an issue with that IN_CONFIG status for some corner cases of git_checkout already, so I'd like to change it so that I can fix those checkout cases. As you probably already know, the submodule APIs have not seen that much usage, so we'll probably have to evolve them...
@KindDragon I'm not quite sure what you're saying is not supported? If status finds a submodule, it should compare the SHA of the current HEAD of the submodule to the SHA in the HEAD of the current branch of the parent repository. That is the extent of what's supported in the current status implementation. We can add flags to enable other behaviors if you can help me pin down what is needed. I can imagine a flag to check if any files in the submodule working directory are dirty (which is useful, but unfortunately somewhat expensive). Or a flag to include the submodule in the status results even if the quick is-HEAD-moved check appears unmodified, so that you can call the deeper |
|
|
|
@dahlbyk |
|
|
|
@nulltoken when you plan merge this pull request? We want submodules API for our project |
|
@KindDragon It's next on my list. Will be done by end of next week. |
|
Hi, I would like to know what the status is of submodule add. Is this already being worked on? And would it be possible to add (and update/stage) a submodule without cloning it? I'm building some code which automatically generates files and stores them in a repository. This also needs to have some submodules included, but there is no need to have those submodules available at the location where the files are getting generated (as the repo will always be cloned and after the clone you of course need to run the submodule update --init). I guess that would be possible by only setting the submodules HEAD, right? And is this feature available in libgit2 and will this also make it into libgit2sharp? Anyway, thanks for the great work on the library in general. It's just so easy to create and update a repository with it 😄 |
| case (int)GitErrorCode.NotFound: | ||
| case (int)GitErrorCode.Exists: | ||
| case (int)GitErrorCode.OrphanedHead: | ||
| break; |
There was a problem hiding this comment.
Would it be more explicit to return null here?
There was a problem hiding this comment.
Returning a null handle felt weird, but I guess we do it other places.
|
I concur with @yorah. Moreover, going down the "snapshot" road, I think I'd prefer to not expose |
| public class Submodule : IEquatable<Submodule> | ||
| { | ||
| private static readonly LambdaEqualityHelper<Submodule> equalityHelper = | ||
| new LambdaEqualityHelper<Submodule>(x => x.Name); |
There was a problem hiding this comment.
Nitpick: Two submodules from two different repos, bearing the same name, shouldn't be considered as equal, should they?
If a Submodule turns into a snapshot, maybe should we add the IndexCommitId, HeadCommitId, WorkdirCommitId as contributors to the equality definition.
There was a problem hiding this comment.
Nitpick: Two submodules from two different repos, bearing the same name, shouldn't be considered as equal, should they?
I suppose not, but I'm not entirely convinced it matters. Can we just document somewhere that "interaction between objects created from different Repository instances could yield unexpected results"?
Another "identity crisis" repro:
using (var repo = Repository.Init(BuildSelfCleaningDirectory().DirectoryPath, true))
using (var repo2 = Repository.Init(BuildSelfCleaningDirectory().DirectoryPath, true))
{
Assert.NotNull(repo.Head);
Assert.Equal(repo.Head, repo2.Head);
}There was a problem hiding this comment.
That said, HeadCommitId can absolutely contribute to equality. Something feels wrong to me about IndexCommitId and WorkdirCommitId contributing to equality, but I can't put a finger on exactly what it is.
|
@dahlbyk 0662c27 is a spike related to the removal of handle from If we don't like the added complexity of having lazy properties, another solution would be to evaluate the properties when we build the Another nitpick while I was at it: I added the Rule suffix to Ignore/Update/FetchRecurse properties (I was confused at first, as Update/Ignore looked like names related to actions that we want to perform). |
|
I like the direction you're going, though I don't like the change in semantics for I'll try to put together a rebound after work.
👍 |
|
@nulltoken Updated 3cffa1f with mention of the Mono issue, so it can probably be cherry-picked standalone. @yorah I'm not a huge fan of how uses of Any other feedback? |
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Oops, I think this missing newline originates from my spike 😊
|
Impressive PR, it's great to see Submodules coming to libgit2sharp!! 👏 👏 |
How about cf574fd? |
Elegant solution! |
Thanks! Squashed! |
|
Wow! You did a fantastic job! 👍 I cherry-picked all of them but the last one. |
| @@ -72,7 +72,7 @@ internal ObjectId TargetId | |||
|
|
|||
| private GitObject RetrieveTreeEntryTarget() | |||
| { | |||
| if (!Type.HasAny(new[]{GitObjectType.Tree, GitObjectType.Blob})) | |||
| if (!Type.HasAny(new[] { GitObjectType.Tree, GitObjectType.Blob, GitObjectType.Commit })) | |||
There was a problem hiding this comment.
I agree that we should not throw in this case. However something bothers me.
Technically, this will allow the creation (few lines below) of a concrete Commit which won't mean anything in the super-project as it technically doesn't exist in this context.
How about adding a new GitLink type. This type would derive from GitObject and would be returned as such.
Provided we go down this road,I can think of some additional topics to cope with
- Inspect any public method accepting a
GitObjectas a parameter and determine how they should behave when being passed aGitLink. - Decide if the
GitLinktype should be internal-only or public
Thoughts?
There was a problem hiding this comment.
I agree that returning a Commit that's not actually a Commit feels a bit strange. We could certainly handle GitObjectType.Commit as a special case here to return a GitLink, but I wonder if it would make more sense to return a new type from libgit2 (e.g. add GIT_OBJ_LINK to git_otype)?
There was a problem hiding this comment.
I wonder if it would make more sense to return a new type from libgit2 (e.g. add
GIT_OBJ_LINKtogit_otype)?
I have the feeling that this proposal may lead to some debate.
How about tweaking this now at the LibGit2Sharp level? When/if this leads to a new entry to git_otype in libgit2, this would later bubble up through failing tests.
There was a problem hiding this comment.
I have the feeling that this proposal may lead to some debate.
You're probably right. I see that git ls-tree returns commit; diverging probably doesn't make sense.
There was a problem hiding this comment.
How about adding a new
GitLinktype. This type would derive fromGitObjectand would be returned as such.
Provided we go down this road,I can think of some additional topics to cope with
- Inspect any public method accepting a
GitObjectas a parameter and determine how they should behave when being passed aGitLink.- Decide if the
GitLinktype should be internal-only or public
Please have a look for yourself, but at a glance I didn't see anywhere that we publicly accept GitObject where we have special cases that depend on the type provided, presumably allowing libgit2 to determine which object types are acceptable.
It would be good to support TreeDefinition.Add(string targetTreeEntryPath, GitLink link) eventually.
There was a problem hiding this comment.
Please have a look for yourself, but at a glance I didn't see anywhere that we publicly accept GitObject where we have special cases that depend on the type provided, presumably allowing libgit2 to determine which object types are acceptable.
You're right. Thanks!
It would be good to support TreeDefinition.Add(string targetTreeEntryPath, GitLink link) eventually.
I strongly agree with you. However; I'm not sure about the API. How would the user create the GitLink in the first place? Beside this, I'm not sure it would be safe to let pick the path by himself.
I wonder if TreeDefinition.Add(Submodule module) would work? Though, I can still think of a corner case where the TreeDefinition would be used to not build a root Tree, but a subTree (which would result in having the path in the .gitmodules file and the path from the root Tree being different)
There was a problem hiding this comment.
How would the user create the
GitLinkin the first place?
Maybe ObjectDatabase.CreateGitLink(ObjectId objectId), similar to existing CreateBlob() and CreateTree()? It would just call the ctor, but the public doesn't need to know that.
I wonder if
TreeDefinition.Add(Submodule module)would work?
That wouldn't really align with the other TreeDefinition APIs, IMO. Perhaps TreeEntryDefinition.From(Submodule module), but that's about as far as I think we'd want to let a high-level concept creep into this low-level API.
|
@dahlbyk Merged! |
|
🎱 |
SubmoduleCollectionthis[name]GetEnumerator()GetEnumerator()- need marshaling helpAdd()- I'd suggest we tackle this with a follow-up PRSubmoduleName,Path,UrlFetchRecurseSubmodules,Ignore,UpdaterulesInit(),Reload(),Save(),Sync()RetrieveStatus()Stage()IndexStage(submodule)(see Add submodule changes to index #220)Tests
While I'm working on setting up proper tests, I thought I'd get this out for feedback...